Skip to content

[ray_client]: Add more retry logic#13478

Merged
ericl merged 16 commits intoray-project:masterfrom
barakmich:client_more_retry
Jan 24, 2021
Merged

[ray_client]: Add more retry logic#13478
ericl merged 16 commits intoray-project:masterfrom
barakmich:client_more_retry

Conversation

@barakmich
Copy link
Contributor

@barakmich barakmich commented Jan 15, 2021

Related issue number

Closes #13446

Checks

  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

@barakmich barakmich marked this pull request as ready for review January 15, 2021 23:17
ericl
ericl reviewed Jan 15, 2021
View reviewed changes
python/ray/util/client/worker.py Outdated
Copy link
Contributor

@ericl ericl Jan 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need three different retry loops for the different stages? Why not have just a single one that tests this last condition?

ericl
ericl requested changes Jan 15, 2021
View reviewed changes
Copy link
Contributor

@ericl ericl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on simplifying

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 15, 2021
@barakmich
Copy link
Contributor Author

barakmich commented Jan 15, 2021 •
edited
Loading

Because it could fail at any one of these stages.

Now, one big retry loop would look like a finite state machine of

while retrying:
   if in step A:
   if in step B:
   if in step C:

But we can't "test this last condition" alone. If this is what you'd prefer, I'll go do that.

@ericl
Copy link
Contributor

ericl commented Jan 16, 2021

Why can't we test the last condition alone? C implies {A, B} are fine right? I think a loop that just tests C would be sufficient. If it's for better error messages, we could also check {A, B} prior to C in the loop.

@ericl
Copy link
Contributor

ericl commented Jan 16, 2021

Btw, it would be good to have a test here

ericl
ericl reviewed Jan 20, 2021
View reviewed changes
@barakmich
add more retry logic
d84fc28 
Change-Id: I4e96d1c4dd7252b754482892597280504a4ba63c
@barakmich
lint
26b7988 
Change-Id: I9b7fae0c7bf15d86c3f3a9bbb6b4eded438975f7
@barakmich
fix formatting and explain each connection error
41d8b29 
Change-Id: I54c890e7e5d6452bcb58312d54c746e5c8e556bc
@barakmich
fmt
9fbd89e 
Change-Id: I278dda94f4bc0c53d93908afdd32011ab1951a13
@barakmich
Make the retry logic a FSM
7fc3f09 
Change-Id: I23dffe65feb4c778985be724f3ab213a61eb2da8
@barakmich
initial cut at a test
88ae27c 
Change-Id: I000bb467e61ca5f2f4e0345d07ca01acf143956f
@barakmich
working tests
30052d1 
Change-Id: I746c4dda014b8b5b7752c07299f9b42951fe6cb8
@barakmich
lint
01b29fa 
Change-Id: I6a0b6d7dccba3d34f6e2c7864909394d016decd3
@barakmich barakmich removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 20, 2021
@barakmich
Remove state machine, catch exceptions
4322ba8 
Change-Id: I9102433a212846af77ac7a23322e35b48d8464b8
@barakmich
remove old comment
41aa1a1 
Change-Id: I443e911ca6e8e6c5fcc91d2ec5a4990d81215c1e
@barakmich
Copy link
Contributor Author

barakmich commented Jan 21, 2021

Sure.

Removed state machine and added test.

ericl
ericl reviewed Jan 22, 2021
View reviewed changes
@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 22, 2021
@barakmich
add delay
a56b71a 
Change-Id: I815b4a0266e4e0fd6331a6b9957309c563c58038
@barakmich barakmich removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 22, 2021
@ericl
Copy link
Contributor

ericl commented Jan 22, 2021 

//python/ray/tests:test_client                                          TIMEOUT in 3 out of 3 in 75.0s
  Stats over 3 runs: max = 75.0s, min = 75.0s, avg = 75.0s, dev = 0.0s
  /home/travis/.cache/bazel/_bazel_travis/b88c129a127452fc94033a29d9f90e20/execroot/com_github_ray_project_ray/bazel-out/k8-opt/testlogs/python/ray/tests/test_client/test.log
  /home/travis/.cache/bazel/_bazel_travis/b88c129a127452fc94033a29d9f90e20/execroot/com_github_ray_project_ray/bazel-out/k8-opt/testlogs/python/ray/tests/test_client/test_attempts/attempt_1.log
  /home/travis/.cache/bazel/_bazel_travis/b88c129a127452fc94033a29d9f90e20/execroot/com_github_ray_project_ray/bazel-out/k8-opt/testlogs/python/ray/tests/test_client/test_attempts/attempt_2.log

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 22, 2021
@barakmich
Merge remote-tracking branch 'origin/master' into client_more_retry
ba0cc85 
Change-Id: I3669ea2734de9e7aad8522a898d3248aa560892a
@barakmich
avoid case where thread may finish before the client connects
885d3a3 
Change-Id: Iae12027d2340e81832305a60a297f85deb8f2919
@barakmich barakmich removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 22, 2021
@barakmich barakmich mentioned this pull request Jan 22, 2021
Merged
6 tasks
@ericl
Copy link
Contributor

ericl commented Jan 22, 2021

Tests / lint still failing

@ericl ericl added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 22, 2021
@barakmich
fix at-least-once and make test_client a medium test
db1eaf4 
Change-Id: Ia3e8df94ea689b38d87f8bbafb5a1142edd7b3e1
@barakmich
Copy link
Contributor Author

barakmich commented Jan 23, 2021

I'm starting to run out of ideas, but we'll see if this goes

@barakmich barakmich removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jan 23, 2021
@barakmich
Copy link
Contributor Author

barakmich commented Jan 23, 2021

I think I figured it out

@barakmich
fix tests, split init test
f0ceac7 
Change-Id: Ie691ed9ac082639c583eec4f6af7578b0841c744
@barakmich
lint
e644c27 
Change-Id: I1f926cd492e9076c77576f388cdbb4ec45de77d1
@barakmich
Copy link
Contributor Author

barakmich commented Jan 23, 2021

This time for sure -- I got the reproduction locally and tracked it down

@ericl ericl merged commit e675e5b into ray-project:master Jan 24, 2021
@barakmich barakmich mentioned this pull request Feb 9, 2021
Closed
2 tasks
fishbone pushed a commit to fishbone/ray that referenced this pull request Feb 16, 2021
@barakmich @fishbone
[ray_client]: Add more retry logic (ray-project#13478)
bc386dd
fishbone added a commit to fishbone/ray that referenced this pull request Feb 16, 2021
@fishbone
Revert "[ray_client]: Add more retry logic (ray-project#13478)"
5d23f96 
This reverts commit bc386dd.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericl ericl ericl approved these changes

Assignees

@ericl ericl

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ray client connect retries not sufficient: sometimes get 502 error

2 participants

@barakmich @ericl